-
-
Notifications
You must be signed in to change notification settings - Fork 25.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MNT Use check_scalar in AdaBoostClassifier #21442
MNT Use check_scalar in AdaBoostClassifier #21442
Conversation
#DataUmbrella sprint |
…nto AdaBoostClassifier_add_check_scaler
def test_adaboost_classifier_params_validation(params, err_type, err_msg): | ||
"""Check the parameters validation in `AdaBoostClassifier`.""" | ||
with pytest.raises(err_type, match=err_msg): | ||
AdaBoostClassifier(**params).fit(X, y_class) # args are from toy sample |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @glemaitre, is it appropriate to test the estimator using the toy sample?
(lines 34-40)
# Toy sample
X = [[-2, -1], [-1, -1], [-1, -2], [1, 1], [1, 2], [2, 1]]
y_class = ["foo", "foo", "foo", 1, 1, 1] # test string class labels
y_regr = [-1, -1, -1, 1, 1, 1]
T = [[-1, -1], [2, 2], [3, 2]]
y_t_class = ["foo", 1, 1]
y_t_regr = [-1, 1, 1]
Would you prefer if I generate different testing data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why this is marked as outdated, but I didn't make any changes to these lines yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this is quite common. We should probably move those into a pytest.fixture
for the full module to make it more explicit but I don't think you need to do that in this PR (it would be an effort for all test files).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! That's good to know, thank you.
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise LGTM
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise, it should be fine.
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
…nto AdaBoostClassifier_add_check_scaler
Thanks @genvalen it looks good. |
Thank you for the feedback. :) |
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Reference Issues/PRs
Addresses #20724
#DataUmbrella
What does this implement/fix? Explain your changes.
Summary of changes to
AdaBoostClassifier
:check_scalar
fromsklearn.utils
to validate the scalar parameters.Progress:
References
1. docs
2. PR #20723
Any other comments?
I am new to these tests. Please make any suggestions. Thank you!